Skip to content

Fix for version converter not applying applicable conversion#2941

Open
LinusJungemann wants to merge 1 commit into
microsoft:mainfrom
LinusJungemann:fix/version-converter
Open

Fix for version converter not applying applicable conversion#2941
LinusJungemann wants to merge 1 commit into
microsoft:mainfrom
LinusJungemann:fix/version-converter

Conversation

@LinusJungemann

Copy link
Copy Markdown

This PR fixes #2938.
Done by changing the way in which the registration of converter functions works.
The version in the decorator represents the target version now.
Because there can only ever be one converter function with same target version and direction combincation, this lookup is unique and will correctly lookup the conversion from the last supported op version, even if multiple model versions are skipped between op versions.

@LinusJungemann

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Paderborn Center for Parallel Computing"

@justinchuby

Copy link
Copy Markdown
Collaborator

I don't think this is the right fix, as from target version we have just the same information. Each function corresponds to one opset pair.

The core issue here is the model supplied is opset 17, which is not supported by this converter. We can add support for that.

@justinchuby justinchuby self-assigned this Jun 18, 2026

@justinchuby justinchuby left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented

@github-project-automation github-project-automation Bot moved this from Todo to In Progress in ONNX Script Review Board Jun 18, 2026
@LinusJungemann

LinusJungemann commented Jun 18, 2026

Copy link
Copy Markdown
Author

This is might be part of the problem, but I think the underlying problem is still how the converters are selected independent of opset version:
Take the Acos operation as an example:
If we have an model with onnx opset 19 (which is supported by the version converter already), then the Acos version will be v7.
If we now upgrade the model to onnx opset 22, then this loop will loop from 19 to 21:

for from_version in range(node_version, self._target_version):

node_version will not be set to 7, because node_version will be set to self._default_onnx_opset, because node.version is not set for a default onnx model.
self._default_onnx_opset is set by
self._default_onnx_opset = _get_onnx_opset_version(model)

which in turn is set to the model version:
return model_version1 or model_version2

If we now have our conversion function registered like this
@register("Acos", node_version=7, up_conversion=True)
then the version converter loop will never find it, because we start at 19. This is wrong, because operator version 7 belongs to model version 7-21. We have to directly upgrade from operator version 7 to operator version 22.
So the upgrade has to only trigger if the version to which we want to upgrade is 22. Because all other upgrades are noops as operator version 7 is still the newest one.

In my opinion, the easiest way to achieve this behavior is not to look at the source version, but the target version. Then we get for an up_conversion registered for opset 22 automatically support for all model source opsets from 7-21 (or 18-21 if the old opset bound for the converter is kept).

I tested the change by implementing an version converter for ReduceLogSumExp in onnx-passes: iksnagreb/onnx-passes#4

An alternative approach would probably be to register the nodes not based on operator version, but based on the last model opset, which they support before an upgrade. So for the example Acos Op, this would then be @register("Acos", needs_upgrade_after_opset=21, up_conversion=True) or something along this line...

@justinchuby

Copy link
Copy Markdown
Collaborator

Thanks for your analysis! Will take a closer look tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Version converter does not apply applicable conversion

2 participants